test: stop leaked-spy cascades across the whole test runner#590
test: stop leaked-spy cascades across the whole test runner#590ndycode wants to merge 3 commits into
Conversation
A test in this file that fails before its inline mockRestore() leaks its fs spy into every later test: a later vi.spyOn on the same method returns the SAME leaked spy, so a passthrough binding captured from it recurses into the new test's own mock (a Maximum call stack crash observed in sandboxed environments where the EACCES baseline tests die mid-run). The top-level afterEach restored env vars but never mocks; vi.restoreAllMocks() there guarantees one failure cannot cascade. No assertion changes; the per-test mockRestore() calls remain as harmless redundancy. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughTest infrastructure adds global and per-test-file ChangesVitest Mock Spy Cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Extends the storage-suite fix to the whole runner: a scan found 16 more test files with the same vulnerability (bare mockRestore() at the end of a test body, no global restore hook), where one mid-test failure leaks a spy that a later vi.spyOn on the same method returns verbatim, sending passthrough bindings into infinite recursion. No suite creates spies in beforeAll, so an after-each restoreAllMocks in the shared global-sandbox setup file is safe and contains the cascade everywhere. https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB
ndycode
changed the title
docs: include token-refresh in the rotation helper cluster description
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/storage.test.ts`:
- Around line 56-60: The test file contains a redundant per-suite call to
vi.restoreAllMocks() that is already handled by the global afterEach hook in
test/helpers/global-sandbox.ts; remove the vi.restoreAllMocks() call from
storage.test.ts to avoid maintenance noise, or if you prefer to keep the
defensive call, add a one-line comment next to vi.restoreAllMocks() referencing
the global afterEach in test/helpers/global-sandbox.ts (and its line range) so
future maintainers understand it is redundant but intentional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be64b33f-4483-4090-ab76-e77572afabb2
📒 Files selected for processing (2)
test/helpers/global-sandbox.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,js,mts,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only with
"type": "module"in package.json; Node >= 18.17 required
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
OAuth callback server uses port 1455; do not hardcode OAuth ports—use existing constants/helpers
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (README.md)
**/*.{js,ts}: Use JSON format for machine-readable output in diagnostic and reporting commands (status, check, report, monitor, why-selected)
Implement interactive terminal dashboard with hotkeys (Up/Down for navigation, Enter for selection, 1-9 for quick switch, / for search, ? for help, Q for back)
Support device authentication flow via --device-auth flag and manual OAuth callback paste fallback via --manual flag for headless environments
Run npm version check during normal forwarded Codex startup and print upgrade notices only on interactive TTY or when CODEX_MULTI_AUTH_DEBUG=1 is set
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
**
⚙️ CodeRabbit configuration file
**: # PROJECT KNOWLEDGE BASEGenerated: 2026-04-25
Commit: a87e005
Validated: 2026-06-10 against commit 98d9819 (repo audit; claims re-checked against the tree, content not regenerated)
Branch: main
Package version: 2.3.0-beta.2OVERVIEW
codex-multi-authis a Codex CLI-first OAuth account manager and optional forwarding wrapper for the official Codex CLI. The installedcodex-multi-authentrypoint handles account-management commands locally,codex-multi-auth-codexforwards official Codex commands through this package's wrapper when explicitly used, and runtime rotation can route live Responses traffic through a localhost account-rotation proxy by default. The plugin-host entrypoint remains exported for compatibility, but the primary product surface is the account manager, optional wrapper, storage, runtime proxy, and repair tooling.STRUCTURE
./ ├── scripts/ │ ├── codex.js # codex-multi-auth-codex wrapper, official CLI forwarder, shadow CODEX_HOME/runtime proxy setup │ ├── codex-multi-auth.js # standalone package CLI entrypoint │ ├── codex-routing.js # auth command and compatibility alias routing │ ├── codex-bin-resolver.js # official Codex binary discovery │ ├── codex-app-router.js # persistent localhost router for packaged Codex app bind │ └── codex-app-launcher.js # reversible user-level app launcher routing helper ├── index.ts # optional plugin-host runtime entry ├── lib/ # core runtime logic (see lib/AGENTS.md) │ ├── auth/ # OAuth flow, PKCE, callback server │ ├── runtime/ # Codex CLI/app integration helpers, app bind, live sync, runtime observability │ ├── request/ # request transform, SSE, failover, backoff │ ├── storage/ # path resolution, migrations, backups, restore, import/export │ ├── codex-cli/ # Codex CLI state sync and writer helpers │ ├── codex-manager/ # command modules and...
Files:
test/helpers/global-sandbox.tstest/storage.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Write Vitest test suites with globals enabled (describe, it, expect)
Maintain 80%+ coverage threshold across statements, branches, functions, and lines
Use removeWithRetry() for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY, EPERM, and ENOTEMPTY errors
Do not rely on dist/ in tests; use source files instead
Do not skip tests without justification
Relax lint rules for test files as configured in eslint.config.js
Files:
test/storage.test.ts
test/**/storage*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test V3 storage, worktree migration, and concurrent load scenarios in storage.test.ts and storage-async.test.ts
Files:
test/storage.test.ts
🧠 Learnings (2)
📚 Learning: 2026-06-04T06:14:18.093Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/scheduling-strategy-config.test.ts:1-1
Timestamp: 2026-06-04T06:14:18.093Z
Learning: In ndycode/codex-multi-auth, do not flag explicit imports from "vitest" (e.g., describe, it, expect, beforeEach/afterEach, etc.) in test files as issues—even if the Vitest config sets `globals: true`. The repo’s established convention is to keep these imports for consistency with neighboring tests; removing them would make files outliers.
Applied to files:
test/storage.test.ts
📚 Learning: 2026-06-04T06:14:24.975Z
Learnt from: ndycode
Repo: ndycode/codex-multi-auth PR: 510
File: test/runtime-rotation-proxy.test.ts:2478-2491
Timestamp: 2026-06-04T06:14:24.975Z
Learning: In ndycode/codex-multi-auth test files (e.g. `test/*.test.ts`), when creating V3 storage fixtures for accounts, it’s an intentional convention to use `as never` for deliberately minimal stored-account objects that only include `refreshToken`, `addedAt`, and `lastUsed`. Do not treat `as never` here as a type-safety problem: optional/other fields are expected to be populated by the runtime during execution, and the cast is used solely to keep the fixture minimal and consistent across existing tests.
Applied to files:
test/storage.test.ts
🔇 Additional comments (1)
test/helpers/global-sandbox.ts (1)
32-41: LGTM!
|
Full-suite validation of the global hook is in (
So the change is a strict improvement: file-level failures stay at the known 8 environment files, and the only name-level deltas are recoveries. Validation evidence retained at Generated by Claude Code |
2 participants
Summary
mockRestore()leaks its spy into every later test of the file.What Changed
The failure mechanics, observed live in sandboxed environments where the EACCES-baseline tests die mid-run:
fs.rename) and crashes before reaching itsmockRestore()(notry/finally).vi.spyOnon the same method — vitest returns the same leaked spy rather than wrapping again.fs.rename.bind(fs)) therefore captures the spy itself, and calling through recurses into the test's own mock — aRangeError: Maximum call stack size exceededcrash with a confusing stack.Two layers land here:
test/storage.test.ts(first commit): the file's top-levelafterEachrestored env vars but never mocks; it now callsvi.restoreAllMocks(), with a comment documenting the mechanism and (per review) its relationship to the global hook. The storage suite is where the cascade was observed.test/helpers/global-sandbox.ts(second commit): a repo scan found 16 more test files with the same shape (baremockRestore()at the end of a test body, no global restore hook). Rather than patching each, the shared setup file — already loaded by every suite — registers one globalafterEach(() => vi.restoreAllMocks()), containing the cascade everywhere. No suite creates spies inbeforeAll(verified by scan), so an after-each restore cannot strand intentional cross-test spies.No assertions change anywhere; the per-test
mockRestore()calls remain as harmless redundancy.Validation
npm run typecheck+npx eslinton both files (pre-commit hook re-runs both)npm test -- test/storage.test.tswith the file-level hook — failure names a strict subset (19) of the 23-name environment baseline block, zero new namesbeforeAll+spyOnco-occurrence search overtest/returns nothing, so no suite relies on a spy surviving past a test--maxWorkers=1): 54 failing names, all in the 58-namedocs/audits/evidence/test-baseline-2026-06-10.txtenvironment baseline, zero new names — and four baseline names now PASS (the storage rotation/backup/artifact tests that were cascade victims, not genuine environment failures). Strict improvement; full name-diff posted as a PR comment.npm run builddeferred to CIDocs and Governance Checklist
Risk and Rollback
vi.restoreAllMocks()only affectsvi.spyOnspies, which every suite already restores per-test on the success path; the hooks merely cover the failure path. The one theoretical hazard (a suite depending on abeforeAllspy across tests) is excluded by scan.https://claude.ai/code/session_01XNtnkLbBiXZxfQQYLMpucB